avoid data race on m_pStream #13899
avoid data race on m_pStream #13899
Conversation
|
m_pStream is meant to be an atomic. I think we can use the null state of this pointer instead of the additional atomic mixxx/src/soundio/sounddeviceportaudio.h Line 62 in 4a35def |
src/soundio/sounddeviceportaudio.cpp
Outdated
| @@ -820,7 +856,9 @@ int SoundDevicePortAudio::callbackProcessDrift( | |||
| //qDebug() << "callbackProcess read:" << (float)readAvailable / outChunkSize << "Buffer empty"; | |||
| } | |||
| } | |||
| return paContinue; | |||
| return m_streamState.load(std::memory_order_acquire) == StreamState::STOP | |||
There was a problem hiding this comment.
This is not used for syncing. Does relax also work? We may avoid the condition to store the return value directly.
There was a problem hiding this comment.
Yeah, I initially stored the return value in an atomic, but IIRC I needed the extra state when starting the streams. But I don't remember so I will revise this.
|
I have made m_pStream atomic and now store the callback return value as atomic directly. Definitely simpler, and it fixes the issues just as well! Thanks for suggesting this, @daschuer |
|
It would also be nice to know if this solves the bugs on linux and windows related with opening and closing device streams, e.g when waking up from sleep and unplugging devices. I am okay with postponing other tsan fixes to post 2.5, but I believe this one should go in. It addresses not only atomic access, but also avoids closing a stream which is still in use in a callback, which seems needlessly risky to me (and the reported data races deep down coreaudio calls show that!) |
This fixes several tsan warnings related with stopping / closing portaudio streams, including #13870.
Apart from access to a non-atomic variable m_pStream, the real problem here was that
readProcess() writeProcess() would take a copy of m_pStream:
and close() would then call Pa_StopStream(m_pStream); Apparently stopping a stream and still accessing it in the callback leads to data races, at least on macOS.
Now, it the portaudio docs I read:
So what I do: in close() I change the (atomic) return value for readProcess and writeProcess from paContinue to paAbort, which will stop the stream. Pa_StopStream can then safely be called and m_pStream set to null.
As a side effect this also addresses a comment in the code about a failed intent to use Pa_AbortStream instead of PaStopStream.
This works well on macOS and I have not seen any portaudio related tsan warnings anymore. But since the platform specific aspects of Portaudio it is important to ensure this works as expected on Windows and Linux.